-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added option to get resolved packages in a requirements.txt file. Fixes #135 #160
Conversation
I was having an issue rebasing in the previous PR #156 . Hence opened this new PR. It is working as expected. Please check. |
@arijitde92 tests are failing in CI, please check. Thanks! |
Hi @TG1999 , After inspecting the cause of test failure, I found that a few test cases are failing but those test cases are not written by me. Please see screenshots below- As you can see, the same tests are failing for all the OSes and none of them are written by me. Is this happening with other PRs too? Or is this happening somehow because of the code I wrote? As I am not able to find any relation between the code I wrote and the tests that are failing. Please guide me on this. |
@arijitde92 please take a pull from latest main branch, main branch is green :) |
Hi @TG1999 , I have pulled the latest main branch in my forked repo as you can see the latest commit in the screenshot below. But still 1 test is failing in CI as shown in below screenshot. |
You have to run 'make valid' command to fix formatting errors |
…#135 Created a new option --resolved-output that finds resolved packages and writes those into the given filename in a typical requirements.txt file format. Also added test test_resolved_cli in test_cli.py Signed-off-by: Arijit De <[email protected]>
Hi @TG1999 , checks are now passing. Let me know if anything else is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arijitde92 thanks++, some nits for your consideration
@@ -54,6 +54,20 @@ def write_output_in_file(output, location): | |||
return output | |||
|
|||
|
|||
def write_resolved_packages(package_list, requirements_file): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arijitde92 please add a separate unit test for this function, separated from CLI logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TG1999 , I have written an unit test in test_cli.py (See here). Do I need to write another unit test elsewhere (maybe in test_utils.py?)?
Also could you please clarify what "separated from CLI logic" means?
I assume that you mean to test for a valid package_list
. In that case do I need to create a dummy package_list
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TG1999 , could you please help in clarifying what kind of unit test you need? And in which while should I write the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arijitde92 yes, please write a test in test_utils.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have written two unit tests in test_utils.py
. Please check.
src/python_inspector/utils.py
Outdated
""" | ||
Write the resolved package names and versions into ``requirements_file_path`` | ||
""" | ||
dependencies = package_list[0]["package_data"][0]["dependencies"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if package_list
is empty? and package_data
does not exist there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented the checks for empty or corrupt package_list
.
…#135 Added unit tests for the function write_resolved_packages in test_utils.py Signed-off-by: Arijit De <[email protected]>
Hi @TG1999 , I have added unit tests as requested and also edited the code as requested. |
@arijitde92 too bad you deleted your repo! |
Created a new option
--resolved-output
that finds resolved packages and writes those into the given filename in a typical requirements.txt file format. Also added test test_resolved_cli in test_cli.py.